Skip to content

ahham - Technical Training Estate module #1171

Open
ahmedhamila wants to merge 12 commits intoodoo:19.0from
odoo-dev:19.0-training-ahham
Open

ahham - Technical Training Estate module #1171
ahmedhamila wants to merge 12 commits intoodoo:19.0from
odoo-dev:19.0-training-ahham

Conversation

@ahmedhamila
Copy link

No description provided.

@robodoo
Copy link

robodoo commented Feb 16, 2026

Pull request status dashboard

@ahmedhamila ahmedhamila changed the title Technical Training Estate module ahham - Technical Training Estate module Feb 16, 2026
Copy link

@ltinel ltinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job, almost nothing to say 🙂

@ltinel
Copy link

ltinel commented Feb 16, 2026

@ahmedhamila Regarding your commits:

  • Could you create one commit per chapter (no need for a FIX commit, you can just squash the fix with the "main" commit).
  • FIX commits are for bug fixes, not linting and formatting (See [1]). You can use CLN or LINT for those (but squashing the commits is better in this case).
  • Could you use imperative style for the first line of your commit message? I.e. "add" instead of "added".
  • The module name (following [ADD]) can be lower case.

[1] https://www.odoo.com/documentation/master/contributing/development/git_guidelines.html

Copy link

@ltinel ltinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work 😄

<field name="bedrooms"/>
<field name="living_area"/>
<field name="facades"/>
<filter name="Available Properties" domain="['|',('state','=','new'),('state','=','offer_received')]"/>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a bit simpler:

Suggested change
<filter name="Available Properties" domain="['|',('state','=','new'),('state','=','offer_received')]"/>
<filter string="Available" name="available" domain="[('state', 'in', ('new', 'offer_received'))]"/>


name = fields.Char(string="Name", required=True)
name = fields.Char(string="Title", required=True)
description = fields.Text(string="Description")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI (no need to change anything): if you don't specify a string parameter to a field, it will use the field name as string (and I believe it correctly converts underscores to spaces and capitalizes the first letter of each word). So if the string parameter and the field name are the same, you don't strictly need to specify it.

Suggested change
description = fields.Text(string="Description")
description = fields.Text()

property_type_id = fields.Many2one("estate.property.type", string="Property Type")
buyer_id = fields.Many2one("res.partner", string="Buyer")
salesman_id = fields.Many2one("res.users", string="Salesman", default=lambda self: self.env.user)
property_tag_ids = fields.Many2many("estate.property.tag")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
property_tag_ids = fields.Many2many("estate.property.tag")
property_tag_ids = fields.Many2many("estate.property.tag", string="Tags")

<field name="name"/>
</h1>
</div>
<field name="property_tag_ids" widget="many2many_tags"/>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this into the oe_title div?


@api.depends("living_area", "garden_area")
def _compute_total_area(self):
for record in self:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's slightly nicer to use a more descriptive name :) (same below)

Suggested change
for record in self:
for property in self:


@api.depends("validity")
def _compute_date_deadline(self):
for record in self:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here (and below)

Suggested change
for record in self:
for offer in self:

@api.depends("validity")
def _compute_date_deadline(self):
for record in self:
if record.create_date:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will prevent the deadline from being set when the record is being created. Could you use a fallback (e.g. today) if create_date isn't set yet? Same for the inverse method.

validity = fields.Integer(string="Validity (days)", default=7)
date_deadline = fields.Date(compute="_compute_date_deadline", inverse="_inverse_date_deadline")

@api.depends("validity")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also depends on create_date :)

@ltinel
Copy link

ltinel commented Feb 17, 2026

FYI, if you scroll to the bottom of your PR, you'll find some Runbot links (i.e. the ci/style and ci/tutorial checks), which are failing. If you click on the links, you'll find some suggestions. Could you implement those to make the checks pass?

Copy link

@ltinel ltinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, great work again :)

Comment on lines 62 to 63
else:
record.state = "cancelled"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for the else condition since there's a raise statement in the if (which shortcuts the method anyway):

Suggested change
else:
record.state = "cancelled"
record.state = "cancelled"

Comment on lines 69 to 70
else:
record.state = "sold"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here:

Suggested change
else:
record.state = "sold"
record.state = "sold"

Comment on lines 35 to 37
for offer in record.property_id.offer_ids:
if offer != record:
offer.status = "refused"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this, you could refuse all offers (including the accepted one), before accepting the offer in question (as you do above):

record.property_id.offer_ids.status = "refused"

But this line should be moved before the record.status = "accepted" statement.

This works because the ORM has special handling for recordsets: this line will set the status of all offers in the recordset to refused.

if record.create_date and record.date_deadline:
record.validity = (record.date_deadline - record.create_date.date()).days

def action_accept_offer(self):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you should also mark the property as having an accepted offer?

record.property_id.state = "offer_accepted"

return True
def action_refuse_offer(self):
for record in self:
record.status = "refused"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should also update the related property, in case the offer was accepted and then refused? (it's a bit of an edge case though 😅 )

Comment on lines 84 to 90
@api.constrains("selling_price")
def _check_selling_price_minimum_ratio(self):
for property in self:
if not property.selling_price:
continue
if float_compare(property.selling_price, property.expected_price * 0.9, precision_digits=2) < 0:
raise ValidationError("The selling price must be at least 90% of the expected price.")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@api.constrains("selling_price")
def _check_selling_price_minimum_ratio(self):
for property in self:
if not property.selling_price:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit risky, it would be safer to use float_is_zero (which allows for precision errors).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants